Skip to content

[nexus-external-api] reorganize per RFD 619#9568

Merged
sunshowers merged 14 commits intomainfrom
sunshowers/spr/nexus-external-api-reorganize-per-rfd-619
Feb 19, 2026
Merged

[nexus-external-api] reorganize per RFD 619#9568
sunshowers merged 14 commits intomainfrom
sunshowers/spr/nexus-external-api-reorganize-per-rfd-619

Conversation

@sunshowers
Copy link
Contributor

This is an extremely large PR, but a generally quite mechanical one. I had Claude Code both write most of the PR and review it for conformance with the RFD -- all tests appear to pass, and I also manually spot-checked the versions crate to ensure it generally made sense.

Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
@sunshowers
Copy link
Contributor Author

@david-crespo one of the things we determined in RFD 619 is that we organize types by semantic area rather than params/views/shared. This makes sense for internal APIs but would love your thoughts on this when applied to the external API (I can put things back in params/views/shared if desired, but wanted your thoughts on the complete migration).

@david-crespo
Copy link
Contributor

I don't think it's a big deal either way. I'll think about it. What I like about importing views and referring to views::Disk etc. is that when you have a bunch of things all called Disk — model, view, params — it’s nice to easily tell which one you’re dealing with. On the other hand there are various other ways to tell. In this case the variable is called params, and the Create in the name also makes it clear, though that's not always the case. The type hover doesn't really tell you:

Screenshot 2025-12-23 at 9 00 04 PM

@david-crespo
Copy link
Contributor

Thinking more about it, I'm fine with it. It's nice to have smaller files and it's nice to have things grouped thematically. If we're worried about confusing models and views in a particular case, we can always import with an alias or something.

Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
@sunshowers sunshowers marked this pull request as ready for review February 16, 2026 22:39
Created using spr 1.3.6-beta.1
/// groups are created automatically.
/// The instance will be automatically added as a member of the specified
/// multicast groups during creation, enabling it to send and receive
/// multicast traffic for those groups.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slightly concerned about some of these changes being accidental reversions of changes that were made on main before this was rebased.

Copy link
Contributor Author

@sunshowers sunshowers Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this is rebased on top of the recent dropshot API manager change (#9546) so doc comments haven't changed for the latest versions of types (they might have changed on older revisions, though — that is a bit trickier to verify, though I can spend some time doing validation if you prefer).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok validation is easier than I thought — dump out all the JSON files as on main (not the blessed versions — regenerated fresh from main), and all the versions as in this PR, and validate no differences.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the different versions might have normalized to share the docstrings of the latest one? But yeah, I realize now that if there are no changes to the OpenAPI schema, any changes here must be to older versions, so it doesn't really matter.

nexus/types/versions/src/local_storage/instance.rs:
    /// The instance will be automatically added as a member of the specified
nexus/types/versions/src/initial/instance.rs:
    /// The instance will be automatically added as a member of the specified
nexus/types/versions/src/pool_selection_enums/instance.rs:
    /// The instance will be automatically added as a member of the specified

Copy link
Contributor

@david-crespo david-crespo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do it. Sorry to all outstanding PRs!

@sunshowers sunshowers merged commit aec72a2 into main Feb 19, 2026
18 checks passed
@sunshowers sunshowers deleted the sunshowers/spr/nexus-external-api-reorganize-per-rfd-619 branch February 19, 2026 22:42
sunshowers added a commit that referenced this pull request Feb 20, 2026
jgallagher added a commit that referenced this pull request Feb 23, 2026
…s-types`

This also addresses some lingering TODOs from #9568 about `From` impls
being defined in the wrong place per RFD 619.
jgallagher added a commit that referenced this pull request Feb 24, 2026
…s-types`

This also addresses some lingering TODOs from #9568 about `From` impls
being defined in the wrong place per RFD 619.
jgallagher added a commit that referenced this pull request Feb 24, 2026
…s-types` (#9909)

This also addresses some lingering TODOs from #9568 about `From` impls
being defined in the wrong place per RFD 619.

Part of #9801.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants